Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix memory leaks due to retained tagify, sortable or tooltipster instances #17483

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Codas
Copy link
Contributor

@Codas Codas commented Nov 25, 2024

This PR fixes memory leaks in character/npc/hazard/party/kingdom/item sheets and various other places.

Any application currently using Sortable, tooltipster or Tagify is currently leaking memory (some references + the entire detached application dom tree) because these utilities register global event handlers or other global references that survive the application elements being detached from the DOM.

The solution to this to use the respective destroy methods these libraries provide. Either .destroy() on the Sortable or Tagify instances or $(element).tooltipster('destroy') for tooltipster.
This is done by keeping references to the active instances and destroying and nulling them when the application is closed, new event handlers are registered or rule element forms are not used anymore.

The memory profiles in the coming examples were done the following way:

  • Reload foundry
  • perform activity once to trigger any lazy allocations that are done only once for each application
  • trigger manual garbage collection in chrome memory profiler
  • start memory profiling
  • do the activity three more times, each time running manual garbage collection after the activity is performed
  • stop memory profiling

Ideal results are: Three spikes in the allocation that reset to a flat line of exactly the same height as before the spike.

These are the results:

NPC Sheet

before PR
00-npc_sheet_before

after PR
00-npc_sheet_after

Character Sheet

before PR
01-character_sheet_before

after PR

01-character_sheet_after

Hazard Sheet _Note: Hazard has to be in "unlocked" mode for tagify to be instantianted and triggere the memory leak_

before PR
02-hazard_sheet_before

after PR
02-hazard_sheet_after

Kingdom Sheet _Note: I was unable to test this one as I don't have access to the Kingmaker Premium Module._

before PR
N/A
after PR
N/A

Party Sheet

before PR
03-party_sheet_before

after PR
03-party_sheet_after

IWR Editor

before PR
04-iwr_editor_before

after PR
04-iwr_editor_after

Compendium Browser _Note: This requires the compendiom browser to be opened with a tab that has tagify instances. The Equipment tab for example._

before PR
05-compendium_browser_before

after PR
05-compendium_browser_after

Pick a Thing Prompt _Note: TODO describe which thing was picked_

before PR
06-pick_a_thing_before

after PR
06-pick_a_thing_after

Encounter Tracker _Note: This occured whenever the listeners were activated again. For testing, the encounter turn was advanced by one step._

before PR
07-encounter_tracker_before

after PR
07-encounter_tracker_after

Note for Rule Elements: This was tested by creating the rule element for a standalone item, then deleting the rule element again without closing the item sheet. Even closing the item sheet however, memory was not freed.

Rule Element Forms: Actor Traits _Note: TODO - how to test?_

before PR
08-actor_traits_ref_before

after PR
08-actor_traits_ref_after

Rule Element Forms: Aura

before PR
09-aura_ref_before

after PR
09-aura_ref_after

Rule Element Forms: Fast Healing _Note: Type has to be set to "regeneration" for memory leak to trigger_

before PR
10-fast_healing_ref_before

after PR
10-fast_healing_ref_after

Rule Element Forms: Roll Note

before PR
11-roll_note_ref_before

after PR
11-roll_note_ref_after

Campaign Feature

before PR
12-campaign_feat_sheet_before

after PR
12-campaign_feat_sheet_after

Deity Sheet

before PR
13-deity_sheet_before

after PR
13-deity_sheet_after

Feat Sheet

before PR
14-feat_sheet_before

after PR
14-feat_sheet_after

Spell Sheet

before PR
15-spell_sheet_before

*after PR
15-spell_sheet_after

Scene Config Sheet

before PR
16-scene_config_before

after PR
16-scene_config_after

Homebrew Language Settings sheet

before PR
17-homebrew_language_before

after PR
17-homebrew_language_after

Skill Check Prompt Macro

before PR
18-request_check_macro_before

after PR
18-request_check_macro_after

@CarlosFdez
Copy link
Collaborator

CarlosFdez commented Nov 25, 2024

What should we be looking at in the profiler? I will be honest and admit that for larger performance concerns outside of time spent, these finer features of the debugger are lost on me, and it seems like the heap is actually larger now overall. I'd be fine with the easiest to understand guide you are aware of.

Are you aware of the code/known issue/etc for all 3 of these libraries and can you link them? One of them being a leak wouldn't be a surprise, but all 3 is a bit wild. What's the likelihood that these global listeners aren't simply reused for all 3 once registered? Or that its memory that hasn't been gc'd yet? seems you accounted for this with manual gc in the profiler.

@Codas
Copy link
Contributor Author

Codas commented Nov 26, 2024

I am sorry, I should have been more precise in my original description.

What you should be looking at in the profiler (I have attached some screenshots of the before/after in the spoiler sections above) is mainly the green line, which represents the number of DOM nodes curently held in memory. Though The orange line (listeners currently in memory) in most cases perfectly correlates to that.
The Blue line in the above screenshots are plain "JS Heap", meaning pure javascript objects like strings, objects etc held in memory. That one is rapidly changing just from running the pixi event loop and is not a useful measurement here.

and it seems like the heap is actually larger now overall

If by that you mean plain JS heap, that one is very volatile and just by running the event loop, some "garbage" will collect, be mainly very shortly lived and then be collected. I have attached screenshot of the memory profile over a 23.5s window. You should see some blue spikes that reset to (mostly) the same low as before.

Screenshot of the plain JS heap over time Bildschirmfoto 2024-11-26 um 06 24 17

Reason for the periodical increase is the pixi/foundry render loop, which allocates some objects, maybe strings, buffers, etc during the rendering of a frame. The drop part of the spike is when the garbage collector kicks in and all the objects that are not referenced anymore are collected.
Now that I look at this in more detail, there might even be an issue with memory management in this core render loop somehow ("low" parts are ever increasing, even after a full garbage collection cycle), but that almost certainly has nothing to do with the pf2e system.

A more detailed analysis of the profile and testing

Lets look a the NPC sheet again. I'll create a new profile and concentrate on the important parts and provide some more details and actual values that are not available just looking at the screenshot. This is how I tested it (fresh reload not visible in the video)

before:
https://github.com/user-attachments/assets/c251c1af-496d-42d2-99de-575901ce07cb
Currently, after opening, then closing the NPC sheet, the number of DOM Nodes, and event listeners kept in memory is ever increasing. From 12,971 at the beginning to 18,767 at the end after a full garbage collecction.

after:
https://github.com/user-attachments/assets/9f88f075-d010-469e-96c6-1852e8288046
These tests were done with the codebase from this PR. In this case, the node count initially increases from 10,443 to 11,907 after closing the npc sheet and GC, but then after each opening, closing, GC cycle, the node count goes from 11,907 up to 14,148 an down again to 11,907 exactly, whithout a single node (or event listener) remaining in memory.

The important part here really is not how much memory is currently in use, how many nodes exactly are being allocated on a fresh reload etc. The important part here is that after opening, then closing a sheet, progressing in the encounter tracker etc, the node and listener count drops to exactly the value as before the operation. Everything else means some memory is being retained forever, ever increasing when of of these operations is being performed. (memory leak)

Regarding the libraries

Are you aware of the code/known issue/etc for all 3 of these libraries and can you link them?

There is no issue at all in the Sortable Library. The tagify Libray has some form of broken auto-cleanup which might warrant an issue or PR on that repository. But skimming over the documentation I would still go as far as to say they behave as intended. Tooltipster is slightly more complicated, but we'll get to that. Lets look at the libraries:

Sortable
Sortable.js keeps a reference of all active sortable instances in a local variable on module level, so instances that are created will never go out of scope (meaning there will always be a reference to that instance starting with the root GC node). The line in question.
The destroy function cleans up these references, making the sortable instance eligible for gargabe collection again.
I can find no reference either in the documentation or code that would suggest that some form of auto cleanup is intended when the DOM node sortable is registered for has become detached.

What's the likelihood that these global listeners aren't simply reused for all 3 once registered?
They are not reused, it's not a question of likelyh

Tagify
At least this line creates a reference by creating a timer (setInterval) which continues running until the interval is manually destroyed. The callback for this interval closes of the "this" variable, keeping it alive indefinitely. The check you see in the second code link (destroying the tagify instance if parentNode is not in the DOM anymore) is never triggered, since the direct parent of the tagify element is alwasys available. When replacing the HTML of the item sheet for example, only the root content node is actually not connected to the dom / has no parent anymore. All child elements are still connected in this detached DOM tree.
To make this library more resilient, the check could be changed to check if a this.DOM.originalInput.closest('body') evaluates to a truthy value... But it is also never documented anywhere that I can find that Tagify has this auto cleanup behavior, so I would argue manually calling destroy when the tagify instance is not needed anymore is the intended behavior of this library.

Tooltipster
I will not repeat the same analysis of what keeps references where for the tooltipster jQuery plugin, mainly because I'm unfamiliar with the jQuery plugin architecture... Fact is however that the situation with cleanup for tooltipster is a bit more complicated. It too provides a sort of destroy method by calling $(element).tooltipster('destroy'), the only issue being that this has to be called before the element tooltipster has been used on is removed from the dom. Reason for that is that in normal cirumstances, removal or replacement of the HTML is done by jQuery via containerElement.html(newHtml) or containerElement.replace(newContainerElement). Both function triger some sort of jQuery cleanup, removing data attributes set by jQuery if I understand correctly. This also removes the tooltipster-ns data attribute that tooltipster itself uses to verify that the instance is still active, throwing an error if that is not the case. Some global references to this tooltipster instance remain however if the element is removed by jQuery, leading to this unfortunate situation of having to destroy tooltipster before it is actually removed from the DOM.

Finally:

Or that its memory that hasn't been gc'd yet? seems you accounted for this with manual gc in the profiler.

You already noticed, but just to clarify that part or my method a bit more. Yes, that is what the manual GC step (brush or trashcan icon in the performance profiler dev tools tab next to the memory checkbox) is used for. This triggeres a full Garbage collection major cycle (or apparently even two full cycles?). This means that both short lived and long lived objects are iterated over, removing ALL memory that is no longer referenced from the root GC node.

I hope I could answer at least some of your questions or concerns. Please do ask further question If something is still unclear :)

src/module/actor/party/kingdom/sheet.ts Outdated Show resolved Hide resolved
src/module/actor/sheet/base.ts Outdated Show resolved Hide resolved
src/module/actor/hazard/sheet.ts Outdated Show resolved Hide resolved
src/module/actor/character/sheet.ts Outdated Show resolved Hide resolved
src/module/actor/party/kingdom/sheet.ts Outdated Show resolved Hide resolved
src/module/actor/party/kingdom/sheet.ts Outdated Show resolved Hide resolved
src/module/actor/sheet/base.ts Outdated Show resolved Hide resolved
src/module/item/campaign-feature/sheet.ts Outdated Show resolved Hide resolved
src/module/item/deity/sheet.ts Outdated Show resolved Hide resolved
src/module/item/feat/sheet.ts Outdated Show resolved Hide resolved
src/module/item/spell/sheet.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@stwlam stwlam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! We're going to wait for a data patch release but will merge soon after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants